ASoC: SOF: Intel: hda: make sure the DMA is started/stopped by checking the RUN bit by zhuyingjiang · Pull Request #997 · thesofproject/linux

zhuyingjiang · 2019-05-31T04:49:47Z

As per the HW recommendation, after setting the RUN bit (start as 1, stop as 0), software must read the bit back to make sure the bit is right set, before modifying related control registers/re-starting the DMA engine.

plbossart

try to fix code style and need to fix assignment of the additional 'run' variable.

plbossart · 2019-05-31T13:31:14Z

 					SOF_HDA_CL_DMA_SD_INT_MASK, 0x0);

+		ret =
+		snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,


it's better you have the function on the previous line. Try to see if you can fix the 80 char issue otherwise it's ok to not follow that rule.

OK, thanks!

plbossart · 2019-05-31T13:32:00Z


 #define HDA_DSP_STREAM_RESET_TIMEOUT		300
 #define HDA_DSP_CL_TRIGGER_TIMEOUT		300
+#define HDA_DSP_STREAM_TRIGGER_TIMEOUT		300


is this the same as legacy or Skylake, if yes add a comment. This looks like copy-paste and not an intentional definition

In the Skylake, it is hard code. OK, I will add a comment to it.

plbossart · 2019-05-31T13:32:25Z

 	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
 	int ret, timeout = HDA_DSP_STREAM_RESET_TIMEOUT;
-	u32 val, mask;
+	u32 val, mask, run;


one variable per line please

OK, thanks!

plbossart · 2019-05-31T13:32:39Z

 				SOF_HDA_SD_CTL_DMA_START, 0);
+
+	ret =
+	snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,


same, function on previous line

OK, thanks!

plbossart · 2019-05-31T13:34:35Z

 	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
 	int ret, timeout = HDA_DSP_STREAM_RESET_TIMEOUT;
-	u32 val, mask;
+	u32 val, mask, run;


run is not set anywhere, have you actually tested this code?

the snd_sof_dsp_read_poll_timeout is a #define defined by @kv2019i :
set by line:
(val) = snd_sof_dsp_read(sdev, bar, offset); \

plbossart · 2019-05-31T13:34:52Z

+	ret =
+	snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
+				      sd_offset, run,
+				      ((run & SOF_HDA_SD_CTL_DMA_START)


how is run set in this patch?

the snd_sof_dsp_read_poll_timeout is a #define defined by @kv2019i :
set by line:
(val) = snd_sof_dsp_read(sdev, bar, offset); \

zhuyingjiang · 2019-06-03T06:44:49Z

@plbossart Updated, with 2 lines exceed 80 chars.

kv2019i

commit language: "to make sure the bit is right set" -> "to make sure the bit is set right"

kv2019i

Looks functionally good. A few editorial comments on commit message, doxygen description and line-wrapping.

kv2019i · 2019-06-03T11:51:21Z

+						    sd_offset, run, ((run &
+						    SOF_HDA_SD_CTL_DMA_START)
+						    ==
+						    SOF_HDA_SD_CTL_DMA_START),


@zhuyingjiang Having just "==" on the line is not readable anymore. I'd recommend not aligning to the opening brace and having a bit more space for the arguemnts. E.g. see call to snd_sof_dsp_read_poll_timeout() in hda-dsp.c:hda_dsp_core_power_down().

@kv2019i OK, thanks!

kv2019i · 2019-06-03T11:55:30Z

+ * Timeout in us, for setting the stream RUN bit, during
+ * start/stop the stream. The time calculate the difference
+ * between write the RUN bit, and read the RUN bit to make
+ * sure it is set.


Suggestion:
""Timeout in us, for setting the stream RUN bit, during
start/stop the stream. The timeout expires if new RUN bit
value cannot be read back within the specified time.""

OK, thanks!

zhuyingjiang · 2019-06-04T01:54:08Z

Updated, thanks @kv2019i for the suggestions!

kv2019i

Thanks! Almost there. One change req to the second commit still:

-During clear the stream status when setting HW parameters,
-the RUN bit must be read back as 0 after setting to 0, to
-make sure the stream is stopped.
+Beforing clearing stream statuses, ensure RUN bit update has taken
+effect by reading the value back.

Otherwise looks good to go!

zhuyingjiang · 2019-06-05T02:17:02Z

@kv2019i Updated, thanks!

kv2019i

Looks good now, thank you!

lgirdwood · 2019-06-05T13:30:10Z

+						sd_offset, run, ((run &
+						SOF_HDA_SD_CTL_DMA_START)
+						== 0x0),
+						HDA_DSP_REG_POLL_INTERVAL_US,


((run & SOF_HDA_SD_CTL_DMA_START) == 0x0), is a lot for a parameter, it's always better to define the variable on an earlier line e.g.
run_value = !(run &SOF_HDA_SD_CTL_DMA_START)
This can be fixed incrementally.

agree with @lgirdwood please add an intermediate variable and simplify this.

@lgirdwood @plbossart @kv2019i
The snd_sof_dsp_read_poll_timeout is a macro #define, not a really function, if using predefined variable as @lgirdwood suggested, the result should be not the same. Because the #define need first get the value of "run", and then the run & SOF_HDA_SD_CTL_DMA_START can be calculated.
@kv2019i Do you agree?

Another solution maybe define a macro for run & SOF_HDA_SD_CTL_DMA_START, as:
1):
#define RUN_VALUE ((run) & SOF_HDA_SD_CTL_DMA_START)

and use RUN_VALUE == SOF_HDA_SD_CTL_DMA_START and RUN_VALUE == 0 as the parameter.

or
2):
#define RUN_DMA_START (((run) & SOF_HDA_SD_CTL_DMA_START) == SOF_HDA_SD_CTL_DMA_START)
#define RUN_DMA_STOP (((run) & SOF_HDA_SD_CTL_DMA_START) == 0x0)

and use as:

ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR, sd_offset, run, RUN_DMA_START, HDA_DSP_REG_POLL_INTERVAL_US, HDA_DSP_STREAM_RUN_TIMEOUT);

What do you think?

@zhuyingjiang I'm ok with the original as well -- this is used elsewhere in the codebase already. I'm not sure how much this helps readability to define more macros. Upto @plbossart and @lgirdwood who made the review comments.

One more option is to use a local variable to describe the mask only:

u32 dma_start = SOF_HDA_SD_CTL_DMA_START; ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR, sd_offset, run, !(run & dma_start), HDA_DSP_REG_POLL_INTERVAL_US,

But yeah, the improvement is limited.

i like this update much better...

OK, thanks @kv2019i , I will update with this solution.

lgirdwood

but please don't put large/complex params as function args.

plbossart

minor changes needed

plbossart · 2019-06-07T14:51:41Z

+						sd_offset, run, ((run &
+						SOF_HDA_SD_CTL_DMA_START)
+						== 0x0),
+						HDA_DSP_REG_POLL_INTERVAL_US,


agree with @lgirdwood please add an intermediate variable and simplify this.

plbossart · 2019-06-07T14:52:26Z

+ * start/stop the stream. The timeout expires if new RUN bit
+ * value cannot be read back within the specified time.
+ */
+#define HDA_DSP_STREAM_RUN_TIMEOUT		300


still doesn't tell me where this value comes from, is this a hardware recommendation or 'same as the other drivers'

@plbossart it is "same as the other drivers"

zhuyingjiang · 2019-06-11T02:17:56Z

@plbossart @lgirdwood @kv2019i Updated, thanks!

As per the HW recommendation, after setting the RUN bit (start as 1, stop as 0), software must read the bit back to make sure the bit is set right, before modifying related control registers/re-starting the DMA engine. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

…ream status Beforing clearing stream statuses, ensure RUN bit update has taken effect by reading the value back. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

plbossart

LGTM, need 2nd reviewer

zhuyingjiang requested review from lgirdwood and plbossart as code owners May 31, 2019 04:49

zhuyingjiang mentioned this pull request May 31, 2019

[APL][GLK] harden: delays after programming registers (and/or loops to check status changes) #777

Closed

zhuyingjiang requested a review from keyonjie May 31, 2019 04:57

plbossart requested changes May 31, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/make-sure-the-RUN-bit-is-set branch from 00a4931 to c6f9ad4 Compare June 3, 2019 06:38

zhuyingjiang requested a review from plbossart June 3, 2019 06:38

kv2019i reviewed Jun 3, 2019

View reviewed changes

kv2019i requested changes Jun 3, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/make-sure-the-RUN-bit-is-set branch from c6f9ad4 to fdcea3e Compare June 4, 2019 01:52

zhuyingjiang requested a review from kv2019i June 4, 2019 01:55

kv2019i requested changes Jun 4, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/make-sure-the-RUN-bit-is-set branch from fdcea3e to dd3e3a9 Compare June 5, 2019 02:16

zhuyingjiang requested a review from kv2019i June 5, 2019 04:22

kv2019i previously approved these changes Jun 5, 2019

View reviewed changes

lgirdwood reviewed Jun 5, 2019

View reviewed changes

lgirdwood previously approved these changes Jun 5, 2019

View reviewed changes

plbossart requested changes Jun 7, 2019

View reviewed changes

zhuyingjiang requested review from kv2019i, lgirdwood and plbossart and removed request for kv2019i, lgirdwood and plbossart June 10, 2019 07:11

zhuyingjiang dismissed stale reviews from lgirdwood and kv2019i via c07c69f June 11, 2019 02:16

zhuyingjiang force-pushed the topic/make-sure-the-RUN-bit-is-set branch from dd3e3a9 to c07c69f Compare June 11, 2019 02:16

zhuyingjiang added 2 commits June 11, 2019 10:26

ASoC: SOF: Intel: hda: make sure RUN bit setting to 0 during clear st…

16dc2ab

…ream status Beforing clearing stream statuses, ensure RUN bit update has taken effect by reading the value back. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang force-pushed the topic/make-sure-the-RUN-bit-is-set branch from c07c69f to 16dc2ab Compare June 11, 2019 02:27

zhuyingjiang mentioned this pull request Jun 11, 2019

Harden APL/GLK hardware support #87

Closed

5 of 7 tasks

plbossart approved these changes Jun 11, 2019

View reviewed changes

kv2019i approved these changes Jun 11, 2019

View reviewed changes

plbossart merged commit 3db5c23 into thesofproject:topic/sof-dev Jun 11, 2019

Conversation

zhuyingjiang commented May 31, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 3, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 4, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 5, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jun 5, 2019 •

edited

Loading

zhuyingjiang Jun 10, 2019 •

edited

Loading